-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Added new analytics parameter for git auto commit #29538
chore: Added new analytics parameter for git auto commit #29538
Conversation
WalkthroughWalkthroughThe recent updates to the codebase involve enhancements to Git-related functionalities, including the tracking of repository URLs in auto-commit events and analytics. Changes also include the introduction of an enum for analytics events and a modification in the user ID hashing logic for analytics. These alterations aim to refine the application's Git integration and analytics reporting, ensuring more precise data capture and streamlined operations. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
/ok-to-test |
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/7177896482. |
private String repoName; | ||
private String authorName; | ||
private String authorEmail; | ||
private String repoUrl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 NOTE
Codebase verification is a beta feature.
The review of the codebase has revealed the following:
- The
repoUrl
field is accessed and used inAutoCommitEventHandlerCEImpl.java
, which aligns with the intended changes. - The test files
AutoCommitEventHandlerImplTest.java
andGitAutoCommitHelperImplTest.java
instantiateAutoCommitEvent
without setting therepoUrl
. This may need to be updated to reflect the new field and ensure that tests cover the new functionality. - The main file
GitAutoCommitHelperImpl.java
also instantiatesAutoCommitEvent
without setting therepoUrl
, which is likely an oversight and should be corrected.
Based on these findings, the following actions are recommended:
-
Verify and update the test files to include the
repoUrl
field in their instantiation ofAutoCommitEvent
. -
Update the instantiation of
AutoCommitEvent
inGitAutoCommitHelperImpl.java
to set therepoUrl
field. -
The test files
AutoCommitEventHandlerImplTest.java
andGitAutoCommitHelperImplTest.java
should be verified and potentially updated to set therepoUrl
when instantiatingAutoCommitEvent
. -
The
GitAutoCommitHelperImpl.java
file should be updated to set therepoUrl
field when instantiatingAutoCommitEvent
.
Analysis chain
The addition of the repoUrl
field to the AutoCommitEvent
class is consistent with the PR objectives and the AI-generated summaries. Ensure that all parts of the codebase that instantiate or use the AutoCommitEvent
class are updated to handle the new repoUrl
field appropriately.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for instances where `AutoCommitEvent` is instantiated to ensure `repoUrl` is being set.
rg 'new AutoCommitEvent' --files-with-matches
# Search for instances where `AutoCommitEvent` is used to ensure `repoUrl` is being handled.
rg 'AutoCommitEvent' --files-with-matches
Length of output: 1218
Script:
#!/bin/bash
# Check the instantiation of `AutoCommitEvent` in the test files to ensure `repoUrl` is being set.
rg 'new AutoCommitEvent' app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/AutoCommitEventHandlerImplTest.java
rg 'new AutoCommitEvent' app/server/appsmith-server/src/test/java/com/appsmith/server/helpers/ce/GitAutoCommitHelperImplTest.java
# Check the instantiation of `AutoCommitEvent` in the main file to ensure `repoUrl` is being set.
rg 'new AutoCommitEvent' app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/GitAutoCommitHelperImpl.java
# Check the usage of `AutoCommitEvent` in the main files to ensure `repoUrl` is being handled.
rg 'AutoCommitEvent' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCEImpl.java
rg 'AutoCommitEvent' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCE.java
rg 'AutoCommitEvent' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/AutoCommitEventHandler.java
rg 'AutoCommitEvent' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/AutoCommitEventHandlerImpl.java
Length of output: 2737
Script:
#!/bin/bash
# Check for the access or modification of the `repoUrl` field in the files where `AutoCommitEvent` is used.
rg 'repoUrl' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCEImpl.java
rg 'repoUrl' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/AutoCommitEventHandlerCE.java
rg 'repoUrl' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/AutoCommitEventHandler.java
rg 'repoUrl' app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/AutoCommitEventHandlerImpl.java
Length of output: 538
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one follow up question on why we removed the .getEventName()
for the analytics?
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/7177896482. |
@AnaghHegde Primary reason was to add some extra parameters for specific git events. In this case, we need to add a new parameter |
Description
A refactor in the analytics events for Git. Also adds isSystemGenerated=false for regular commits.
PR fixes following issue(s)
Fixes #26769
Media
Type of change
Testing
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity:
Test Plan Approved
label after Cypress tests were reviewedTest Plan Approved
label after JUnit tests were reviewedSummary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Tests